Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #13021: cmdline: validate option --std #6723

Closed
wants to merge 7 commits into from

Conversation

ludviggunne
Copy link
Contributor

Command line parsing fails with an error on commands such as

$ cppcheck --std=c++19 somefile.cpp

@ludviggunne
Copy link
Contributor Author

The tests seem to fail because they use '--std=cplusplus11'. Should I change the parsing to accept the 'cplusplus' prefix in addition to 'c++'?

@chrchr-github
Copy link
Collaborator

The tests seem to fail because they use '--std=cplusplus11'. Should I change the parsing to accept the 'cplusplus' prefix in addition to 'c++'?

No, just turn the TODO_ASSERT_EQUALS into a regular ASSERT_EQUALS.

@chrchr-github
Copy link
Collaborator

So this line now results in unknownMacro errors: ./cppcheck -q --template=selfcheck --error-exitcode=1 --library=cppcheck-lib --library=qt -D__CPPCHECK__ -D__GNUC__ -DQT_VERSION=0x060000 -DQ_MOC_OUTPUT_REVISION=68 -DQT_CHARTS_LIB -DQT_MOC_HAS_STRINGDATA --enable=unusedFunction --exception-handling -rp=. --project=cmake.output.notest/compile_commands.json --suppressions-list=.selfcheck_unused_suppressions --inline-suppr
I wonder what that compile_commands.json looks like.

lib/standards.h Outdated Show resolved Hide resolved
@danmar
Copy link
Owner

danmar commented Aug 26, 2024

sorry for late reply.

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I basically like this so make the CI happy.

For your info you can run a specific test class like this:

 testrunner TestClass

You can run a specific test function like this:

testrunner TestClass::testcase

@ludviggunne
Copy link
Contributor Author

Ok, seems to work now. I'm still unsure about what to do with the 'gnu++11' options generated by CMake in the self check though. Would it be appropriate to sed it to c++11 in the CI script as a quick fix?

@danmar
Copy link
Owner

danmar commented Aug 27, 2024

Ok, seems to work now. I'm still unsure about what to do with the 'gnu++11' options generated by CMake in the self check though. Would it be appropriate to sed it to c++11 in the CI script as a quick fix?

I think that users might have to sed sometimes. however it wouldn't hurt to handle gnu++11 in the importproject directly. using --std=gnu++11 on the command line should still be an error but if we see it an compile_commands.json imho it would make sense to handle it like "c++11".

@chrchr-github
Copy link
Collaborator

Shouldn't we be consistent here, i.e. handle gnu++11 everywhere or not at all?

@ludviggunne
Copy link
Contributor Author

Perhaps handle it if a certain cli option is set (e.g. —allow-gnu)? Adds flexibility but prevents any misunderstandings.

@danmar
Copy link
Owner

danmar commented Aug 27, 2024

Shouldn't we be consistent here, i.e. handle gnu++11 everywhere or not at all?

my thinking was that why would you really want to use --std=gnu++11 on the command line instead of c++11? I don't see the use case really.

in a compile_commands.json file it's a different story. That is not a cppcheck option but a gcc option. And I assume it's not very uncommon.

@ludviggunne
Copy link
Contributor Author

I have a commit that handles gnu++XX as c++XX in command files, should I push?

@chrchr-github
Copy link
Collaborator

I have a commit that handles gnu++XX as c++XX in command files, should I push?

That's fine with me.

@firewave
Copy link
Collaborator

Sorry for the late reply but I have been out sick and still not fully recovered.

This is possibly all wrong. The whole knowledge about what is a valid standard already exists in simplecpp and that needs to be leveraged. Since it is handled differently in Cppcheck I added the TODO. I cannot really tell if I just didn't have the time to implement it (and then forgot) or if there was something more problematic preventing this.

This also needs tests for the project import.

@danmar
Copy link
Owner

danmar commented Aug 29, 2024

The whole knowledge about what is a valid standard already exists in simplecpp and that needs to be leveraged.

hmm you mean to utilize simplecpp::getCppStdString and simplecpp::getCStdString right? That sounds pretty interesting to me however it does not seem trivial directly. Maybe we should split up the simplecpp functions but that should be done upstream of course not here in cppcheck repo.

@firewave
Copy link
Collaborator

hmm you mean to utilize simplecpp::getCppStdString and simplecpp::getCStdString right?

Yes, at least the knowledge. I did not have a look yet. IIRC there is also some inconsistencies with the later on usage. I might not have slapped a TODO on everything.

I think this should done incrementally. Start with the CLI parameter bailing out based on the existing logic. Then plumping in the simplecpp logic and afterwards address the project import part (as already mentioned that needs some test coverage - and maybe quite some of it).

@danmar
Copy link
Owner

danmar commented Aug 29, 2024

I think this should done incrementally. Start with the CLI parameter bailing out based on the existing logic.

yes that is acceptable to me also. we don't have to change the standards.h or importproject at all it's enough with cmdlineparser and testcmdlineparser..

@ludviggunne
Copy link
Contributor Author

I think this should done incrementally. Start with the CLI parameter bailing out based on the existing logic.

yes that is acceptable to me also. we don't have to change the standards.h or importproject at all it's enough with cmdlineparser and testcmdlineparser..

I suppose this should be done in a new pull request?

@firewave
Copy link
Collaborator

I suppose this should be done in a new pull request?

That is up to you. You can keep this and just remove the stuff that is deemed out of scope for the first step.

@ludviggunne
Copy link
Contributor Author

I created a new pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants